Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

THRIFT-4134-Fix-remaining-undefined-behavior-invalid.patch #1222

Closed
wants to merge 3 commits into from
Closed

THRIFT-4134-Fix-remaining-undefined-behavior-invalid.patch #1222

wants to merge 3 commits into from

Conversation

Jens-G
Copy link
Member

@Jens-G Jens-G commented Mar 26, 2017

No description provided.

jbapple-cloudera
jbapple-cloudera suggested changes Mar 26, 2017
View reviewed changes
build/docker/scripts/ubsan.sh
@@ -15,7 +15,7 @@ export CXX=clang++-3.8
# undefined casting, aka "vptr".
#
# TODO: fix undefined vptr behavior and turn this option back on.
export CFLAGS="-fsanitize=undefined -fno-sanitize-recover=undefined -fno-sanitize=vptr"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll still want to include -fno-sanitize-recover=undefined, I think. When that option is set, a binary will crash on undefined behavior. When it is not set, the binary will print an error and continue.

Having it set is preferable, I think, so that undefined behavior causes the tests to fail.

Copy link
Member Author

@Jens-G Jens-G Mar 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just used your proposal. Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have been more explicit. For finding vptr errors, it's good to remove -fno-sanitize-recover=undefined. But for checking that there are none, it's good to have -fno-sanitize-recover=undefined.

If this patch fixes some, but not all, of the vptr errors, I would suggest leaving CFLAGS as it is in master. If it fixes all vptr errors, I think the way you have it now is optimal.

compiler/cpp/src/thrift/generate/t_json_generator.cc
t_type* etype = ((t_list*)ttype)->get_elem_type();
write_key_and_string("elemTypeId", get_type_name(etype));
write_type_spec_object("elemType", etype);
} else if (ttype->is_set()) {
t_type* etype = ((t_set*)ttype)->get_elem_type();
write_key_and_string("elemTypeId", get_type_name(etype));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: these lines are duplicated from above. They can be refactored by setting etype in a conditional and putting 282 and 283 below that.

compiler/cpp/src/thrift/generate/t_xml_generator.cc
t_type* etype = ((t_list*)ttype)->get_elem_type();
write_element_start("elemType");
write_type(etype);
write_element_end();
} else if (type == "set") {
t_type* etype = ((t_set*)ttype)->get_elem_type();
write_element_start("elemType");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: duplicate lines (see above)

Copy link
Member Author

@Jens-G Jens-G Mar 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know they are duplicates because I duplicated them. One could argue that it is more readable but sure, we can have a conditional. Or we could refactor it into a method and include the map case.

@Jens-G
Copy link
Member Author

Jens-G commented Mar 26, 2017 

void t_json_generator::write_type_spec(t_type* ttype) {
  ttype = ttype->get_true_type();

  write_string(get_type_name(ttype));

  if (ttype->is_struct() || ttype->is_xception()) {
    write_key_and_string("class", get_qualified_name(ttype));
    return;
  }

  if (ttype->is_map()) {
    t_type* ktype = ((t_map*)ttype)->get_key_type();
    t_type* vtype = ((t_map*)ttype)->get_val_type();
    write_key_and_string("keyTypeId", get_type_name(ktype));
    write_key_and_string("valueTypeId", get_type_name(vtype));
    write_type_spec_object("keyType", ktype);
    write_type_spec_object("valueType", vtype);
    return;
  }

  t_type* etype;
  if (ttype->is_list()) {
    etype = ((t_list*)ttype)->get_elem_type();
  } else if (ttype->is_set()) {
    etype = ((t_set*)ttype)->get_elem_type();
  } else {
    etype = NULL;
  }
  if( etype != NULL) {
    write_key_and_string("elemTypeId", get_type_name(etype));
    write_type_spec_object("elemType", etype);
    return;
  }
}

Mmmh. I think I go with the duplicates. Feel free to make it better.

@Jens-G
Copy link
Member Author

Jens-G commented Mar 26, 2017

No, the intention is to patch everything. A few things are still open, but I will update the PR until we have it all.

@jbapple-cloudera
Copy link
Contributor

How about

void t_json_generator::write_type_spec(t_type* ttype) {
  ttype = ttype->get_true_type();

  write_string(get_type_name(ttype));

  if (ttype->is_struct() || ttype->is_xception()) {
    write_key_and_string("class", get_qualified_name(ttype));
    return;
  }

  if (ttype->is_map()) {
    t_type* ktype = ((t_map*)ttype)->get_key_type();
    t_type* vtype = ((t_map*)ttype)->get_val_type();
    write_key_and_string("keyTypeId", get_type_name(ktype));
    write_key_and_string("valueTypeId", get_type_name(vtype));
    write_type_spec_object("keyType", ktype);
    write_type_spec_object("valueType", vtype);
    return;
  }

  t_type* etype = NULL;
  if (ttype->is_list()) {
    etype = ((t_list*)ttype)->get_elem_type();
  } else if (ttype->is_set()) {
    etype = ((t_set*)ttype)->get_elem_type();
  }

  if( etype != NULL) {
    write_key_and_string("elemTypeId", get_type_name(etype));
    write_type_spec_object("elemType", etype);
  }
}

Another alternative is to do what your other patch is doing and have t_set and t_list inherit from a base type that has a get_elem_type() virtual method.

@Jens-G
Copy link
Member Author

Jens-G commented Mar 26, 2017

To be honest, my intention is to fix the actual problem, not to refactor the whole app.

@Jens-G Jens-G closed this Mar 26, 2017
@Jens-G Jens-G deleted the THRIFT-4134 branch March 26, 2017 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbapple-cloudera jbapple-cloudera jbapple-cloudera requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Jens-G @jbapple-cloudera